jobparser for crd TrainingJob #14

m3ngyang · 2018-03-17T08:25:43Z

New parser reuses the most codes of the old one, except it returns an integrated CRD TrainingJob instead of separated ReplicaSet or Job.

seiriosPlus · 2018-03-20T11:40:14Z

pkg/updater/jobparser.go

+		corev1.EnvVar{Name: "PADDLE_INIT_NUM_GRADIENT_SERVERS", Value: strconv.Itoa(job.Spec.Trainer.MinInstance)},
+		corev1.EnvVar{Name: "PADDLE_INIT_NUM_PASSES", Value: strconv.Itoa(job.Spec.Passes)},
+		corev1.EnvVar{Name: "PADDLE_INIT_USE_GPU", Value: needGPU},
+		corev1.EnvVar{Name: "LD_LIBRARY_PATH", Value: "/usr/local/cuda/lib64"},


LD_LIBRARY_PATH 的值需要根据 volumeMounts中 nvidia-libraries 的值进行更新

这个如何更新呢？

…arser

m3ngyang · 2018-03-23T05:54:58Z

@helinwang @typhoonzero plz help review the pr.

typhoonzero

If these files are quite the same to the original ones, can we remove the old files?

typhoonzero · 2018-03-24T13:21:53Z

pkg/updater/jobparser.go

+func parseToPserver(job *paddlev1.TrainingJob) *v1beta1.ReplicaSet {
+	replicas := int32(job.Spec.Pserver.MinInstance)
+	command := make([]string, 2, 2)
+	// FIXME: refine these part.


FIXME comment should add your id like FIXME(typhoonzero).

typhoonzero · 2018-03-24T13:26:02Z

pkg/updater/jobparser.go

+		corev1.EnvVar{Name: "TRAINERS", Value: strconv.Itoa(job.Spec.Trainer.MinInstance)},
+		corev1.EnvVar{Name: "PSERVERS", Value: strconv.Itoa(job.Spec.Pserver.MinInstance)},
+		corev1.EnvVar{Name: "ENTRY", Value: job.Spec.Trainer.Entrypoint},
+		// FIXME: TOPOLOGY deprecated


same as above

helinwang · 2018-03-27T21:56:34Z

pkg/apis/paddlepaddle/v1/trainingjob.go

+
+func (s *TrainingJob) String() string {
+	b, _ := json.MarshalIndent(s, "", "   ")
+	return string(b[:])


I think string(b) will suffice.
Btw, just curious why String method is required, I normally use fmt.Sprintf("%+v", foo)
From https://golang.org/pkg/fmt/#hdr-Printing:

%v the value in a default format when printing structs, the plus flag (%+v) adds field names %#v a Go-syntax representation of the value %T a Go-syntax representation of the type of the value %% a literal percent sign; consumes no value

helinwang · 2018-03-27T22:03:20Z

pkg/updater/jobparser.go

+}
+
+// DefaultJobParser implement a basic JobParser.
+type DefaultJobParser int


Maybe

type DefaultJobParser struct {}

is more extensible.

In this case, using int as the underlying type is a little strange, since it allows int-operators such as parserA + parserB, which has no real meaning. Plus, a := DefaultJobParser(1) is no different from b := DefaultJobParser(2), which is strange.

I think struct{} is a good default if the receiver's methods don't use the value of the receiver. E.g., in this case, method Validate and other methods don't use the value of the receiver p.

helinwang · 2018-03-27T22:12:11Z

pkg/updater/jobparser.go

+type DefaultJobParser int
+
+// Validate updates default values for the added job and validates the fields.
+func (p *DefaultJobParser) Validate(job *paddlev1.TrainingJob) error {


Maybe change name to SetDefaultAndValidate since this method not only validates, but also sets default values.

helinwang · 2018-03-27T22:13:16Z

pkg/updater/jobparser.go

+	if !job.Spec.FaultTolerant && job.Elastic() {
+		return errors.New("max-instances should equal to min-instances when fault_tolerant is disabled")
+	}
+	// TODO: add validations.


Could you please add id for todo? E.g., // TODO(helin): ...

helinwang · 2018-03-27T23:19:08Z

pkg/updater/jobparser.go

+
+// JobParser is a interface can parse given simple TrainingJob struct to
+// an integrated TrainingJob
+type JobParser interface {


Do we need this interface?

My personal rule of thumb is not to use interface unless necessary, here are the common reasons for using interface:

to avoid circular package dependency.

multiple implementations of an interface.

dependency inversion principle: it is about inverting dependency across different packages, so only necessary when multiple packages are involved.

I typically start with one implementation without interface, when there are actually multiple implementations, create an interface. Typically there will be never a need for multiple implementations.

Some related articles: https://medium.com/@cep21/what-accept-interfaces-return-structs-means-in-go-2fe879e25ee8

helinwang · 2018-03-27T23:21:38Z

pkg/updater/jobparser.go

+
+// JobParser is a interface can parse given simple TrainingJob struct to
+// an integrated TrainingJob
+type JobParser interface {


The "parser" name is a little confusing, typically parser parses some raw data into a data structure. But judging from the methods, no raw data is involved.

Maybe I have this confusion because ParseToTrainingJob takes a *paddlev1.TrainingJob and return a *paddlev1.TrainingJob, should we make it to take a job.Spec rather than job?

Maybe we can combine Validate(job *paddlev1.TrainingJob) error and ParseToTrainingJob(job *paddlev1.TrainingJob) *paddlev1.TrainingJob into a package level function NewTrainingJob(*Spec) (*paddlev1.TrainingJob, error).

m3ngyang · 2018-03-30T11:27:32Z

I fixed some problems mentioned above. As for the following questions:

If these files are quite the same to the original ones, can we remove the old files?

I will submit a pr to remove outdated files. @typhoonzero

should we make it to take a job.Spec rather than job?

To make the most of previous structure and codes, I adopt Job and ReplicaSet structure as sub-resource of TrainingJob. But I leave a TODO in the definition of TrainingJob to simplify the structure. @helinwang

typhoonzero

LGTM!

wangkuiyi · 2018-04-03T02:01:58Z

pkg/apis/paddlepaddle/v1/trainingjob.go

@@ -0,0 +1,32 @@
+package v1


The package name here should be

package edl // import "github.com/edl/v1"

instead of

package v1

Please take this file from the Go client library of Google API as a reference.

wangkuiyi · 2018-04-03T02:08:15Z

pkg/apis/paddlepaddle/v1/trainingjob.go

@@ -0,0 +1,32 @@
+package v1


I noticed that in many of our Go repos, there is a directory pkg. I have never seen such a convention in other Go repos -- not in any well-known libraries like Google's API's Go client, not Go lang's repo itself.

This convention would make it hard for other Go projects to import ours. The authors have to write:

import "github.com/paddlepaddle/edl/pkg/apis/paddlepaddle/v1"

Please be aware that there are two paddlepaddle in the above import path. Ths is pretty redundant.

To make this import path concise and clear, we might want

import "github.com/paddlepaddle/edl/v1"

To achive this, we should have training_job.go in the /v1, instead of /pkg/apis/paddlepaddle/v1.

pkg directory is widely used in many go repos like https://github.com/coreos/etcd, https://github.com/kubernetes/kubernetes, and the intention is to hide internal implement. I think we can keep the pkg directory and put cmd/edl to edl is more general. Currently since edl does not have any API provided for users to call, we don't have to expose apis directory, /pkg/apis/paddlepaddle/v1 is only for internal use and call the k8s API.

wangkuiyi · 2018-04-03T02:08:50Z

pkg/apis/paddlepaddle/v1/trainingjob.go

@@ -0,0 +1,32 @@
+package v1


Please follow Go's convention and rename this file from trainingjob.go into training_job.go.

wangkuiyi · 2018-04-03T02:09:17Z

pkg/apis/paddlepaddle/v1/types.go

@@ -55,6 +55,7 @@ type TrainingJobSpec struct {
 	Passes            int                  `json:"passes,omitempty"`
 	Volumes           []corev1.Volume      `json:"volumes"`
 	VolumeMounts      []corev1.VolumeMount `json:"VolumeMounts"`
+	//TODO simplify the structure of sub-resource(mengyang)


// TODO(mengyang): Simplify the structure of sub-resource.

wangkuiyi · 2018-04-03T02:10:34Z

pkg/updater/jobparser.go

+	}
+}
+
+// -----------------------------------------------------------------------


Go's comment should follow Godoc's convention: https://blog.golang.org/godoc-documenting-go-code.

wangkuiyi · 2018-04-03T02:13:36Z

pkg/updater/jobparser.go

+	apiresource "k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	paddlev1 "github.com/paddlepaddle/edl/pkg/apis/paddlepaddle/v1"


import "github.com/paddlepaddle/edl/v1"

wangkuiyi · 2018-04-03T02:14:00Z

pkg/updater/jobparser.go

+}
+
+// setDefaultAndValidate updates default values for the added job and validates the fields.
+func setDefaultAndValidate(job *paddlev1.TrainingJob) error {


paddlev1 => edl

And for all the rest namings.

wangkuiyi · 2018-04-03T02:17:18Z

pkg/updater/trainingJobUpdater.go

@@ -2,15 +2,16 @@ package updater

 import (
 	"fmt"
+	"reflect"
+	"time"
+
 	log "github.com/golang/glog"


These import paths are not sorted lexically. This violates the Go code style convention. You might need to configure your favorite editor to call either gofmt or goimports whenever you save the .go files.

Are you using Emacs? If so, a very convenient configuration is https://github.com/helinwang/go-emacs.

wangkuiyi · 2018-04-03T02:18:14Z

pkg/updater/trainingJobUpdater.go

@@ -2,15 +2,16 @@ package updater

 import (
 	"fmt"
+	"reflect"
+	"time"
+
 	log "github.com/golang/glog"
 	padv1 "github.com/paddlepaddle/edl/pkg/apis/paddlepaddle/v1"


padv1 "github.com/paddlepaddle/edl/pkg/apis/paddlepaddle/v1"

=>

"github.com/paddlepaddle/edl/v1"

m3ngyang · 2018-04-04T12:23:27Z

thx for your review @wangkuiyi and I have fixed some code style convention problem you mentioned above, and I agree with @typhoonzero so I keep the current directory structs.

helinwang · 2018-04-10T18:58:19Z

pkg/updater/jobparser.go

+// parseToTrainer parse TrainingJob to a kubernetes job resource.
+func parseToTrainer(job *paddlev1.TrainingJob) *batchv1.Job {
+	replicas := int32(job.Spec.Trainer.MinInstance)
+	command := make([]string, 2)


make([]string, 2) creates a slice which is never used. var command []string is suffice.

jobparser for crd TrainingJob

5448520

m3ngyang requested review from helinwang and typhoonzero March 17, 2018 08:28

seiriosPlus reviewed Mar 20, 2018

View reviewed changes

seiriosPlus approved these changes Mar 20, 2018

View reviewed changes

seiriosPlus and others added 5 commits March 20, 2018 20:06

Merge branch 'develop_CRD' into tj-parser

1a57114

Merge remote-tracking branch 'upstream/develop_CRD' into tj-parser

213ee4a

fix conflicts

eec86bb

Merge branch 'tj-parser' of https://github.com/m3ngyang/edl into tj-p…

06ac7da

…arser

fix metaobject of pserver

d1038b5

typhoonzero reviewed Mar 24, 2018

View reviewed changes

helinwang reviewed Mar 27, 2018

View reviewed changes

redefine generation function of job parser

65dd456

typhoonzero approved these changes Mar 30, 2018

View reviewed changes

wangkuiyi requested changes Apr 3, 2018

View reviewed changes

wangkuiyi reviewed Apr 3, 2018

View reviewed changes

fix code style convention problem

499e1fb

helinwang reviewed Apr 10, 2018

View reviewed changes

fix definition of command variable

39f1380

wangkuiyi approved these changes Apr 11, 2018

View reviewed changes

m3ngyang merged commit 57937d1 into elasticdeeplearning:develop_CRD Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jobparser for crd TrainingJob #14

jobparser for crd TrainingJob #14

m3ngyang commented Mar 17, 2018

seiriosPlus Mar 20, 2018

m3ngyang Mar 20, 2018

m3ngyang commented Mar 23, 2018

typhoonzero left a comment

typhoonzero Mar 24, 2018

typhoonzero Mar 24, 2018

helinwang Mar 27, 2018

helinwang Mar 27, 2018

helinwang Mar 27, 2018

helinwang Mar 27, 2018

helinwang Mar 27, 2018 •

edited

Loading

helinwang Mar 27, 2018

m3ngyang commented Mar 30, 2018 •

edited

Loading

typhoonzero left a comment

wangkuiyi Apr 3, 2018 •

edited

Loading

wangkuiyi Apr 3, 2018

typhoonzero Apr 3, 2018

wangkuiyi Apr 3, 2018

m3ngyang Apr 8, 2018

wangkuiyi Apr 3, 2018

wangkuiyi Apr 3, 2018

wangkuiyi Apr 3, 2018

wangkuiyi Apr 3, 2018

wangkuiyi Apr 3, 2018

wangkuiyi Apr 3, 2018

m3ngyang commented Apr 4, 2018

helinwang Apr 10, 2018

m3ngyang Apr 11, 2018

jobparser for crd TrainingJob #14

jobparser for crd TrainingJob #14

Conversation

m3ngyang commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3ngyang commented Mar 23, 2018

typhoonzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3ngyang commented Mar 30, 2018 • edited Loading

typhoonzero left a comment

Choose a reason for hiding this comment

wangkuiyi Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3ngyang commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Mar 27, 2018 •

edited

Loading

m3ngyang commented Mar 30, 2018 •

edited

Loading

wangkuiyi Apr 3, 2018 •

edited

Loading